feat(speakers): add unique activities count endpoints for speakers and submitters#543
Open
mulldug wants to merge 1 commit into
Open
feat(speakers): add unique activities count endpoints for speakers and submitters#543mulldug wants to merge 1 commit into
mulldug wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis PR adds two new OAuth2-protected GET endpoints for counting unique summit activities for speakers and submitters, with optional filter support. It includes repository implementations, API controllers with OpenAPI documentation, routing, database seeding via both seeder and migration, comprehensive tests, and supporting infrastructure updates. ChangesActivities Count Endpoints
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-543/ This page is automatically updated on each push to this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
tests/oauth2/OAuth2SummitSubmittersApiTest.php (1)
264-301: ⚡ Quick winStrengthen count assertions to catch regressions earlier.
These tests only assert
count >= 0. Add an integer-type assertion and a filtered-vs-unfiltered comparison (filtered <= unfiltered) to validate behavior, not just shape.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/oauth2/OAuth2SummitSubmittersApiTest.php` around lines 264 - 301, Update the assertions in testGetCurrentSummitSubmittersActivitiesCountWithAcceptedPresentations (and the sibling unfiltered test if present) to validate type and relational behavior: assert the returned $data->count is an integer (use assertIsInt or assertTrue(is_int(...))) and then perform an additional request to get the unfiltered count (call OAuth2SummitSubmittersApiController@getSubmittersActivitiesCount with the same $params but without the 'filter' entry), decode that response and assert that the filtered count is less than or equal to the unfiltered count ($filteredCount <= $unfilteredCount); use the existing $response/$content/json_decode flow and $this->action helper to obtain the unfiltered result.app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php (1)
432-497: ⚖️ Poor tradeoffConsider extracting the duplicated filter definitions.
The filter field definitions (lines 442-464) and validation rules (lines 468-490) are duplicated across
getSpeakers,getSpeakersCSV,send, and this newgetSpeakersActivitiesCountmethod. Extracting these to private methods or class constants would improve maintainability and ensure consistency when filter fields are added or modified.♻️ Example refactor
private function getSpeakerFilterDefinition(): array { return [ 'id' => ['=='], 'not_id' => ['=='], 'first_name' => ['=@', '@@', '=='], 'last_name' => ['=@', '@@', '=='], 'email' => ['=@', '@@', '=='], 'full_name' => ['=@', '@@', '=='], 'member_id' => ['=='], 'member_user_external_id' => ['=='], 'has_accepted_presentations' => ['=='], 'has_alternate_presentations' => ['=='], 'has_rejected_presentations' => ['=='], 'presentations_track_id' => ['=='], 'presentations_track_group_id' => ['=='], 'presentations_selection_plan_id' => ['=='], 'presentations_type_id' => ['=='], 'presentations_title' => ['=@', '@@', '=='], 'presentations_abstract' => ['=@', '@@', '=='], 'presentations_submitter_full_name' => ['=@', '@@', '=='], 'presentations_submitter_email' => ['=@', '@@', '=='], 'has_media_upload_with_type' => ['=='], 'has_not_media_upload_with_type' => ['=='], ]; } private function getSpeakerFilterValidationRules(): array { return [ 'id' => 'sometimes|integer', 'not_id' => 'sometimes|integer', 'first_name' => 'sometimes|string', 'last_name' => 'sometimes|string', 'email' => 'sometimes|string', 'full_name' => 'sometimes|string', 'member_id' => 'sometimes|integer', 'member_user_external_id' => 'sometimes|integer', 'has_accepted_presentations' => 'sometimes|required|string|in:true,false', 'has_alternate_presentations' => 'sometimes|required|string|in:true,false', 'has_rejected_presentations' => 'sometimes|required|string|in:true,false', 'presentations_track_id' => 'sometimes|integer', 'presentations_track_group_id' => 'sometimes|integer', 'presentations_selection_plan_id' => 'sometimes|integer', 'presentations_type_id' => 'sometimes|integer', 'presentations_title' => 'sometimes|string', 'presentations_abstract' => 'sometimes|string', 'presentations_submitter_full_name' => 'sometimes|string', 'presentations_submitter_email' => 'sometimes|string', 'has_media_upload_with_type' => 'sometimes|integer', 'has_not_media_upload_with_type' => 'sometimes|integer', ]; }Then use:
$filter = FilterParser::parse(Request::input('filter'), $this->getSpeakerFilterDefinition()); // ... if (!is_null($filter)) { $filter->validate($this->getSpeakerFilterValidationRules()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php` around lines 432 - 497, The filter field definitions and validation rules duplicated in getSpeakersActivitiesCount should be extracted into reusable helpers (e.g., private methods or class constants) and reused across getSpeakers, getSpeakersCSV, send and getSpeakersActivitiesCount; create methods like getSpeakerFilterDefinition() and getSpeakerFilterValidationRules() returning the arrays shown in the comment, then replace the inline arrays in getSpeakersActivitiesCount (the FilterParser::parse call and the $filter->validate call) and the same spots in getSpeakers, getSpeakersCSV and send to call those helper methods instead.tests/oauth2/OAuth2SummitSpeakersApiTest.php (1)
1938-1968: 💤 Low valueConsider comparing filtered vs unfiltered count for consistency.
The test
testGetCurrentSummitSpeakersActivitiesCountFilteredBySelPlan(lines 1903-1936) establishes a useful pattern: it fetches both unfiltered and filtered counts, then asserts the filtered count is less than or equal to the unfiltered count. This test only fetches the filtered count and asserts it's greater than zero. For consistency and stronger validation, consider adding an unfiltered baseline call and comparing the results.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/oauth2/OAuth2SummitSpeakersApiTest.php` around lines 1938 - 1968, The test method testGetCurrentSummitSpeakersActivitiesCountWithAcceptedPresentations should also fetch an unfiltered baseline using the same controller action getSpeakersActivitiesCount and same summit id (but without the filter param), parse its JSON count, then assert the filtered count is > 0 and that filtered_count <= unfiltered_count to mirror the pattern in testGetCurrentSummitSpeakersActivitiesCountFilteredBySelPlan; locate and update the test method testGetCurrentSummitSpeakersActivitiesCountWithAcceptedPresentations to perform the extra request, decode the response, and add the additional assertion comparing counts.app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSubmittersApiController.php (1)
539-604: ⚖️ Poor tradeoffConsider extracting the duplicated filter definitions.
The filter field definitions (lines 549-571) and validation rules (lines 575-597) are duplicated across
getAllBySummit,getAllBySummitCSV,send, and this newgetSubmittersActivitiesCountmethod. Extracting these to a private method or class constant would improve maintainability and ensure consistency when filter fields are added or modified.♻️ Example refactor
private function getSubmitterFilterDefinition(): array { return [ 'id' => ['=='], 'not_id' => ['=='], 'first_name' => ['=@', '@@', '=='], 'last_name' => ['=@', '@@', '=='], 'email' => ['=@', '@@', '=='], 'full_name' => ['=@', '@@', '=='], 'member_id' => ['=='], 'member_user_external_id' => ['=='], 'has_accepted_presentations' => ['=='], 'has_alternate_presentations' => ['=='], 'has_rejected_presentations' => ['=='], 'presentations_track_id' => ['=='], 'presentations_selection_plan_id' => ['=='], 'presentations_type_id' => ['=='], 'presentations_title' => ['=@', '@@', '=='], 'presentations_abstract' => ['=@', '@@', '=='], 'presentations_submitter_full_name' => ['=@', '@@', '=='], 'presentations_submitter_email' => ['=@', '@@', '=='], 'is_speaker' => ['=='], 'has_media_upload_with_type' => ['=='], 'has_not_media_upload_with_type' => ['=='], ]; } private function getSubmitterFilterValidationRules(): array { return [ 'id' => 'sometimes|integer', 'not_id' => 'sometimes|integer', 'first_name' => 'sometimes|string', 'last_name' => 'sometimes|string', 'email' => 'sometimes|string', 'full_name' => 'sometimes|string', 'member_id' => 'sometimes|integer', 'member_user_external_id' => 'sometimes|integer', 'has_accepted_presentations' => 'sometimes|string|in:true,false', 'has_alternate_presentations' => 'sometimes|string|in:true,false', 'has_rejected_presentations' => 'sometimes|string|in:true,false', 'presentations_track_id' => 'sometimes|integer', 'presentations_selection_plan_id' => 'sometimes|integer', 'presentations_type_id' => 'sometimes|integer', 'presentations_title' => 'sometimes|string', 'presentations_abstract' => 'sometimes|string', 'presentations_submitter_full_name' => 'sometimes|string', 'presentations_submitter_email' => 'sometimes|string', 'is_speaker' => 'sometimes|string|in:true,false', 'has_media_upload_with_type' => 'sometimes|integer', 'has_not_media_upload_with_type' => 'sometimes|integer', ]; }Then use:
$filter = FilterParser::parse(Request::input('filter'), $this->getSubmitterFilterDefinition()); // ... $filter->validate($this->getSubmitterFilterValidationRules());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSubmittersApiController.php` around lines 539 - 604, The duplicated submitter filter definitions and validation rules used in getSubmittersActivitiesCount (and also in getAllBySummit, getAllBySummitCSV, send) should be extracted into reusable methods or constants; add private methods like getSubmitterFilterDefinition() and getSubmitterFilterValidationRules() that return the arrays shown in the review, then replace the inline arrays in getSubmittersActivitiesCount with FilterParser::parse(Request::input('filter'), $this->getSubmitterFilterDefinition()) and $filter->validate($this->getSubmitterFilterValidationRules()); update the other methods (getAllBySummit, getAllBySummitCSV, send) to call these new methods to remove duplication and keep behavior identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Http/Middleware/OAuth2BearerAccessTokenRequestValidator.php`:
- Around line 175-177: The origin check in
OAuth2BearerAccessTokenRequestValidator uses str_contains on
token_info->getAllowedOrigins(), which allows partial matches; update the logic
to perform exact origin matching: normalize the incoming $origin (trim trailing
slash), split token_info->getAllowedOrigins() into discrete origins (e.g., by
comma) and check for strict equality (or use in_array) against the normalized
origin when getApplicationType() === 'JS_CLIENT', ensuring null/empty $origin
still fails.
In `@app/Repositories/Summit/DoctrineMemberRepository.php`:
- Around line 669-686: The current code in DoctrineMemberRepository builds
$memberIds by materializing $qb->getQuery()->getArrayResult() and then passes
that array into the COUNT query (member_ids), which can blow up for large
summits; replace this two-step approach with a single SQL that counts distinct
presentations for submitters from the summit using a subquery or JOIN instead of
an IN-list (e.g. COUNT(DISTINCT p.ID) FROM Presentation p INNER JOIN SummitEvent
se ON se.ID = p.ID WHERE se.SummitID = :summit_id AND p.CreatedByID IN (SELECT
DISTINCT m.ID FROM Member m INNER JOIN <relevant_table> ... WHERE SummitID =
:summit_id) or use EXISTS: WHERE EXISTS (SELECT 1 FROM <submitter_relation> s
WHERE s.MemberID = p.CreatedByID AND s.SummitID = :summit_id)); update the call
to executeQuery to only bind the single summit_id parameter (remove member_ids
and ArrayParameterType usage) and keep the query execution within
DoctrineMemberRepository (target symbols: $qb, getArrayResult, $memberIds,
Presentation p, SummitEvent se, executeQuery).
In `@app/Repositories/Summit/DoctrineSpeakerRepository.php`:
- Around line 731-755: The code in DoctrineSpeakerRepository materializes all
speaker IDs via $qb->getQuery()->getArrayResult() into $speakerIds and then uses
IN (:speaker_ids) in a raw SQL count, which is brittle for large sets; instead
modify the count SQL to embed the speaker selection as a SQL subquery (or reuse
the QueryBuilder as a subselect) so you never pass a large PHP array — replace
the IN (:speaker_ids) clauses with EXISTS/subselects that reference the same
speaker-selection logic (derived from $qb) and execute the count with a single
query using bound scalar parameters (e.g., :summit_id) rather than an
ArrayParameterType for speaker ids.
In `@database/seeders/ConfigSeeder.php`:
- Around line 38-42: The seeder currently calls non-existent methods
evictEntityRegions() and evictQueryRegions() on $l2Cache; replace them with the
proper Doctrine Cache API calls by invoking $l2Cache->evictQueryCache() for
query cache, and for entities either call
$l2Cache->evictEntityRegion($entityClass) in a loop over your entity class names
(e.g., from metadata) or call $l2Cache->evictEntityRegion(null) to evict all
entity regions; update the code in ConfigSeeder where $l2Cache is used to
perform these correct calls.
---
Nitpick comments:
In
`@app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php`:
- Around line 432-497: The filter field definitions and validation rules
duplicated in getSpeakersActivitiesCount should be extracted into reusable
helpers (e.g., private methods or class constants) and reused across
getSpeakers, getSpeakersCSV, send and getSpeakersActivitiesCount; create methods
like getSpeakerFilterDefinition() and getSpeakerFilterValidationRules()
returning the arrays shown in the comment, then replace the inline arrays in
getSpeakersActivitiesCount (the FilterParser::parse call and the
$filter->validate call) and the same spots in getSpeakers, getSpeakersCSV and
send to call those helper methods instead.
In
`@app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSubmittersApiController.php`:
- Around line 539-604: The duplicated submitter filter definitions and
validation rules used in getSubmittersActivitiesCount (and also in
getAllBySummit, getAllBySummitCSV, send) should be extracted into reusable
methods or constants; add private methods like getSubmitterFilterDefinition()
and getSubmitterFilterValidationRules() that return the arrays shown in the
review, then replace the inline arrays in getSubmittersActivitiesCount with
FilterParser::parse(Request::input('filter'),
$this->getSubmitterFilterDefinition()) and
$filter->validate($this->getSubmitterFilterValidationRules()); update the other
methods (getAllBySummit, getAllBySummitCSV, send) to call these new methods to
remove duplication and keep behavior identical.
In `@tests/oauth2/OAuth2SummitSpeakersApiTest.php`:
- Around line 1938-1968: The test method
testGetCurrentSummitSpeakersActivitiesCountWithAcceptedPresentations should also
fetch an unfiltered baseline using the same controller action
getSpeakersActivitiesCount and same summit id (but without the filter param),
parse its JSON count, then assert the filtered count is > 0 and that
filtered_count <= unfiltered_count to mirror the pattern in
testGetCurrentSummitSpeakersActivitiesCountFilteredBySelPlan; locate and update
the test method
testGetCurrentSummitSpeakersActivitiesCountWithAcceptedPresentations to perform
the extra request, decode the response, and add the additional assertion
comparing counts.
In `@tests/oauth2/OAuth2SummitSubmittersApiTest.php`:
- Around line 264-301: Update the assertions in
testGetCurrentSummitSubmittersActivitiesCountWithAcceptedPresentations (and the
sibling unfiltered test if present) to validate type and relational behavior:
assert the returned $data->count is an integer (use assertIsInt or
assertTrue(is_int(...))) and then perform an additional request to get the
unfiltered count (call
OAuth2SummitSubmittersApiController@getSubmittersActivitiesCount with the same
$params but without the 'filter' entry), decode that response and assert that
the filtered count is less than or equal to the unfiltered count ($filteredCount
<= $unfilteredCount); use the existing $response/$content/json_decode flow and
$this->action helper to obtain the unfiltered result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 36745d1c-f076-4af9-b8a0-83be313aee9a
📒 Files selected for processing (17)
.env.example.gitignoreapp/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.phpapp/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSubmittersApiController.phpapp/Http/Middleware/OAuth2BearerAccessTokenRequestValidator.phpapp/ModelSerializers/Summit/SummitSerializer.phpapp/Models/Foundation/Main/Repositories/IMemberRepository.phpapp/Models/Foundation/Summit/Repositories/ISpeakerRepository.phpapp/Repositories/Summit/DoctrineMemberRepository.phpapp/Repositories/Summit/DoctrineSpeakerRepository.phpdatabase/seeders/ApiEndpointsSeeder.phpdatabase/seeders/ConfigSeeder.phpdocker-compose.override.testing.ymlroutes/api_v1.phptests/SubmitterRepositoryTest.phptests/oauth2/OAuth2SummitSpeakersApiTest.phptests/oauth2/OAuth2SummitSubmittersApiTest.php
mulldug
force-pushed
the
feature/speakers-submitters-activities-count
branch
from
ca6dbae to
bd000e6
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-543/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/Repositories/Summit/DoctrineSpeakerRepository.php (1)
711-713: ⚡ Quick winPush presentation de-duplication into SQL for the speaker branch.
speakerQbcan return duplicatep.idrows due to thep.speakersjoin; deduping earlier reduces result size and PHP work with no behavior change.Proposed change
- $speakerQb = $this->getEntityManager()->createQueryBuilder() - ->select("p.id") + $speakerQb = $this->getEntityManager()->createQueryBuilder() + ->select("DISTINCT p.id") ->from('models\summit\Presentation', 'p') ->join('p.speakers', '__cnt_assign') ->join('__cnt_assign.speaker', 'e')Also applies to: 741-744
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Repositories/Summit/DoctrineSpeakerRepository.php` around lines 711 - 713, The query builder in DoctrineSpeakerRepository producing $speakerQb can return duplicate p.id rows because of the p.speakers join; modify the query to deduplicate at the SQL level (e.g., change the select to request unique ids or add a GROUP BY) so it returns distinct presentation ids (use DISTINCT p.id or group by p.id) to reduce result size and PHP-side work; apply the same fix to the other occurrence of $speakerQb-like code around the later block (the similar select at lines ~741-744).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/Repositories/Summit/DoctrineSpeakerRepository.php`:
- Around line 711-713: The query builder in DoctrineSpeakerRepository producing
$speakerQb can return duplicate p.id rows because of the p.speakers join; modify
the query to deduplicate at the SQL level (e.g., change the select to request
unique ids or add a GROUP BY) so it returns distinct presentation ids (use
DISTINCT p.id or group by p.id) to reduce result size and PHP-side work; apply
the same fix to the other occurrence of $speakerQb-like code around the later
block (the similar select at lines ~741-744).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6fac9124-d47b-4e92-be67-726afb96df68
📒 Files selected for processing (17)
.env.example.gitignoreapp/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.phpapp/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSubmittersApiController.phpapp/Http/Middleware/OAuth2BearerAccessTokenRequestValidator.phpapp/ModelSerializers/Summit/SummitSerializer.phpapp/Models/Foundation/Main/Repositories/IMemberRepository.phpapp/Models/Foundation/Summit/Repositories/ISpeakerRepository.phpapp/Repositories/Summit/DoctrineMemberRepository.phpapp/Repositories/Summit/DoctrineSpeakerRepository.phpdatabase/seeders/ApiEndpointsSeeder.phpdatabase/seeders/ConfigSeeder.phpdocker-compose.override.testing.ymlroutes/api_v1.phptests/SubmitterRepositoryTest.phptests/oauth2/OAuth2SummitSpeakersApiTest.phptests/oauth2/OAuth2SummitSubmittersApiTest.php
✅ Files skipped from review due to trivial changes (4)
- .env.example
- docker-compose.override.testing.yml
- .gitignore
- app/Models/Foundation/Main/Repositories/IMemberRepository.php
🚧 Files skipped from review as they are similar to previous changes (10)
- app/Http/Middleware/OAuth2BearerAccessTokenRequestValidator.php
- database/seeders/ConfigSeeder.php
- app/ModelSerializers/Summit/SummitSerializer.php
- routes/api_v1.php
- tests/oauth2/OAuth2SummitSubmittersApiTest.php
- app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSubmittersApiController.php
- app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php
- tests/oauth2/OAuth2SummitSpeakersApiTest.php
- database/seeders/ApiEndpointsSeeder.php
- tests/SubmitterRepositoryTest.php
smarcet
reviewed
Jun 1, 2026
smarcet
reviewed
Jun 1, 2026
smarcet
reviewed
Jun 1, 2026
smarcet
reviewed
Jun 1, 2026
smarcet
reviewed
Jun 1, 2026
Collaborator
Test coverage for the new activities-count endpointsThe new count endpoints are exercised, but the assertions are happy-path only and one repository test is effectively a no-op in CI. Worth tightening before merge:
See the inline comments for the specific spots. |
smarcet
reviewed
Jun 1, 2026
| $summit_repository = EntityManager::getRepository(Summit::class); | ||
|
|
||
| $summit = $summit_repository->find(3401); | ||
| if (is_null($summit)) $this->markTestSkipped('Summit 3401 not in test DB'); |
Collaborator
There was a problem hiding this comment.
This test no-ops on a clean CI database: summit 3401 won't exist, so it hits markTestSkipped and provides zero coverage for getUniqueActivitiesCountBySummit. Please drive it off the seeded test summit (as the API tests do) so the repository method is actually exercised in CI rather than skipped.
smarcet
reviewed
Jun 1, 2026
| $data = json_decode($content); | ||
| $this->assertNotNull($data); | ||
| $this->assertTrue(isset($data->count)); | ||
| $this->assertGreaterThan(0, $data->count); |
Collaborator
There was a problem hiding this comment.
Happy-path only: count > 0 would still pass if moderators are double-counted, the array_unique dedup is broken, or the join is wrong. Please assert an exact expected count against a known fixture — ideally a presentation with multiple assigned speakers plus a moderator — so the speaker/moderator union + dedup is actually verified, not just "non-zero".
smarcet
reviewed
Jun 1, 2026
|
|
||
| // Take the union of both result sets and count distinct presentation IDs. | ||
| // Presentation counts per summit are small (<1000) so PHP-side deduplication is fine. | ||
| $speakerIds = array_column($speakerQb->getQuery()->getScalarResult(), 'id'); |
Collaborator
There was a problem hiding this comment.
This count path materializes every (presentation, speaker) and (presentation, moderator) row into PHP via getScalarResult() and dedups with array_unique. The "presentation counts per summit are small (<1000)" assumption is unenforced and degrades for large summits, while still executing two full filtered queries.
This is now also inconsistent with DoctrineMemberRepository::getUniqueActivitiesCountBySummit, which was already moved to a SQL subquery. Please do the same here — take the union of the two presentation-id sets and COUNT(DISTINCT p.id) in DQL/SQL instead of pulling all ids into PHP.
smarcet
requested changes
Jun 1, 2026
mulldug
force-pushed
the
feature/speakers-submitters-activities-count
branch
from
bd000e6 to
621283c
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-543/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/Repositories/Summit/DoctrineSpeakerRepository.php (1)
712-724: ⚡ Quick winConsider replacing cross-join with explicit JOINs or UNION for clarity.
The two
FROMclauses (lines 714-715) create a cartesian product of Presentation × PresentationSpeaker, then filter via theEXISTSsubquery andOR p.moderator = econdition. While functionally correct and properly deduplicated viaCOUNT(DISTINCT p.id), this cross-join approach processes N×M intermediate rows before filtering.For better readability and potentially improved query plan, consider using:
- Option 1: Explicit INNER JOIN via PresentationSpeakerAssignment + LEFT JOIN for moderators
- Option 2: UNION of two simpler queries (one for assignments, one for moderators)
However, if the current approach performs acceptably in production and the query plan shows adequate filtering, this optimization can be deferred.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Repositories/Summit/DoctrineSpeakerRepository.php` around lines 712 - 724, The query in DoctrineSpeakerRepository building $countQb uses two FROM clauses (Presentation p and PresentationSpeaker e) producing a cross-join; replace this with an explicit join-based or union-based approach to avoid the cartesian product: update the QueryBuilder logic that constructs the COUNT query (the block creating $countQb selecting COUNT(DISTINCT p.id)) to either 1) JOIN Presentation to PresentationSpeakerAssignment (or PresentationSpeaker via a proper JOIN) and LEFT JOIN the moderator relationship so the WHERE clause can reference those joins without a second FROM, or 2) build two simpler queries (one for presentations matched via PresentationSpeakerAssignment and one for p.moderator) and UNION them before counting distinct p.id; ensure you keep the same parameter binding for :summit and preserve the EXISTS/p.moderator semantics while removing the cross-join between Presentation and PresentationSpeaker.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/SubmitterRepositoryTest.php`:
- Line 85: The assertion assertIsArray($submitterIds) is too weak; update the
test in SubmitterRepositoryTest.php to assert that $submitterIds actually
contains data from the seeded summit fixture — for example replace or supplement
assertIsArray($submitterIds) with assertNotEmpty($submitterIds) or
assertGreaterThan(0, count($submitterIds)), and optionally assert expected
values (e.g., assertContains/ assertEquals on a known seeded submitter ID)
against the $submitterIds array so the repository call (the test that calls the
repository method returning $submitterIds) fails when no IDs are returned.
---
Nitpick comments:
In `@app/Repositories/Summit/DoctrineSpeakerRepository.php`:
- Around line 712-724: The query in DoctrineSpeakerRepository building $countQb
uses two FROM clauses (Presentation p and PresentationSpeaker e) producing a
cross-join; replace this with an explicit join-based or union-based approach to
avoid the cartesian product: update the QueryBuilder logic that constructs the
COUNT query (the block creating $countQb selecting COUNT(DISTINCT p.id)) to
either 1) JOIN Presentation to PresentationSpeakerAssignment (or
PresentationSpeaker via a proper JOIN) and LEFT JOIN the moderator relationship
so the WHERE clause can reference those joins without a second FROM, or 2) build
two simpler queries (one for presentations matched via
PresentationSpeakerAssignment and one for p.moderator) and UNION them before
counting distinct p.id; ensure you keep the same parameter binding for :summit
and preserve the EXISTS/p.moderator semantics while removing the cross-join
between Presentation and PresentationSpeaker.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d83e26c5-ffda-47b8-bda0-54ccc5fab375
📒 Files selected for processing (17)
.env.example.gitignoreapp/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.phpapp/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSubmittersApiController.phpapp/Http/Middleware/OAuth2BearerAccessTokenRequestValidator.phpapp/ModelSerializers/Summit/SummitSerializer.phpapp/Models/Foundation/Main/Repositories/IMemberRepository.phpapp/Models/Foundation/Summit/Repositories/ISpeakerRepository.phpapp/Repositories/Summit/DoctrineMemberRepository.phpapp/Repositories/Summit/DoctrineSpeakerRepository.phpdatabase/migrations/config/Version20260603000000.phpdatabase/seeders/ApiEndpointsSeeder.phpdatabase/seeders/ConfigSeeder.phproutes/api_v1.phptests/SubmitterRepositoryTest.phptests/oauth2/OAuth2SummitSpeakersApiTest.phptests/oauth2/OAuth2SummitSubmittersApiTest.php
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- .env.example
🚧 Files skipped from review as they are similar to previous changes (8)
- app/Http/Middleware/OAuth2BearerAccessTokenRequestValidator.php
- app/ModelSerializers/Summit/SummitSerializer.php
- database/seeders/ConfigSeeder.php
- app/Repositories/Summit/DoctrineMemberRepository.php
- app/Models/Foundation/Summit/Repositories/ISpeakerRepository.php
- app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php
- database/seeders/ApiEndpointsSeeder.php
- tests/oauth2/OAuth2SummitSubmittersApiTest.php
| $submitterIds = $submitter_repository->getSubmittersIdsBySummit($summit, new PagingInfo(1, 5), $filter, $order); | ||
| $submitterIds = $submitter_repository->getSubmittersIdsBySummit(self::$summit, new PagingInfo(1, 5), $filter, $order); | ||
|
|
||
| self::assertIsArray($submitterIds); |
There was a problem hiding this comment.
assertIsArray alone is too weak for repository coverage.
This now passes even when no submitter IDs are returned, which reduces regression detection. Add a content assertion (e.g., non-empty) tied to the seeded summit fixture.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/SubmitterRepositoryTest.php` at line 85, The assertion
assertIsArray($submitterIds) is too weak; update the test in
SubmitterRepositoryTest.php to assert that $submitterIds actually contains data
from the seeded summit fixture — for example replace or supplement
assertIsArray($submitterIds) with assertNotEmpty($submitterIds) or
assertGreaterThan(0, count($submitterIds)), and optionally assert expected
values (e.g., assertContains/ assertEquals on a known seeded submitter ID)
against the $submitterIds array so the repository call (the test that calls the
repository method returning $submitterIds) fails when no IDs are returned.
Comment on lines
+102
to
+103
| self::assertIsInt($filteredCount); | ||
| self::assertLessThanOrEqual($totalCount, $filteredCount); |
There was a problem hiding this comment.
Filtered/unfiltered count assertion is reversed.
The inequality is flipped: filtered count should be <= total count, not the other way around.
Suggested fix
- self::assertLessThanOrEqual($totalCount, $filteredCount);
+ self::assertLessThanOrEqual($filteredCount, $totalCount);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| self::assertIsInt($filteredCount); | |
| self::assertLessThanOrEqual($totalCount, $filteredCount); | |
| self::assertIsInt($filteredCount); | |
| self::assertLessThanOrEqual($filteredCount, $totalCount); |
…d submitters
- Add GET /summits/{id}/speakers/all/events/count and GET /summits/{id}/submitters/all/events/count controller methods with OpenAPI attributes and route registrations
- Implement getUniqueActivitiesCountBySummit in DoctrineSpeakerRepository and DoctrineMemberRepository using two-stage DQL→raw SQL COUNT(DISTINCT)
- Add interface declarations to ISpeakerRepository and IMemberRepository
- Register both endpoints in ApiEndpointsSeeder with ReadSummitData scopes
- Add PHPUnit tests for both HTTP endpoints and repository-level method
- Fix null-guard bug in DoctrineMemberRepository::applyExtraJoins
mulldug
force-pushed
the
feature/speakers-submitters-activities-count
branch
from
621283c to
d9cf735
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-543/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/SubmitterRepositoryTest.php`:
- Around line 91-103: The test currently only checks types and bounds for
$totalCount and $filteredCount, which lets join fan-out or filter bugs pass;
replace the loose assertions with exact expected values for the seeded summit
fixture by asserting $totalCount === <EXPECTED_TOTAL> and $filteredCount ===
<EXPECTED_FILTERED> (use the concrete integers from the fixture), keeping the
same call sites getUniqueActivitiesCountBySummit(self::$summit, null) and
getUniqueActivitiesCountBySummit(self::$summit, $filter) and the same
FilterParser::parse([... "is_speaker" => ['=='] ...]) setup so the
distinct-count and is_speaker filter behavior is validated precisely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4fea2d3c-6f09-4517-b98c-e8de695a1cbe
📒 Files selected for processing (16)
.env.example.gitignoreapp/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.phpapp/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSubmittersApiController.phpapp/ModelSerializers/Summit/SummitSerializer.phpapp/Models/Foundation/Main/Repositories/IMemberRepository.phpapp/Models/Foundation/Summit/Repositories/ISpeakerRepository.phpapp/Repositories/Summit/DoctrineMemberRepository.phpapp/Repositories/Summit/DoctrineSpeakerRepository.phpdatabase/migrations/config/Version20260603000000.phpdatabase/seeders/ApiEndpointsSeeder.phpdatabase/seeders/ConfigSeeder.phproutes/api_v1.phptests/SubmitterRepositoryTest.phptests/oauth2/OAuth2SummitSpeakersApiTest.phptests/oauth2/OAuth2SummitSubmittersApiTest.php
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- .env.example
- app/ModelSerializers/Summit/SummitSerializer.php
🚧 Files skipped from review as they are similar to previous changes (11)
- app/Models/Foundation/Summit/Repositories/ISpeakerRepository.php
- routes/api_v1.php
- app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSubmittersApiController.php
- tests/oauth2/OAuth2SummitSubmittersApiTest.php
- database/seeders/ApiEndpointsSeeder.php
- app/Models/Foundation/Main/Repositories/IMemberRepository.php
- tests/oauth2/OAuth2SummitSpeakersApiTest.php
- database/seeders/ConfigSeeder.php
- app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersApiController.php
- app/Repositories/Summit/DoctrineSpeakerRepository.php
- app/Repositories/Summit/DoctrineMemberRepository.php
Comment on lines
+91
to
+103
| $totalCount = $submitter_repository->getUniqueActivitiesCountBySummit(self::$summit, null); | ||
| self::assertIsInt($totalCount); | ||
| self::assertGreaterThanOrEqual(0, $totalCount); | ||
|
|
||
| $filter = FilterParser::parse( | ||
| ["filter" => "is_speaker==false"], | ||
| ["is_speaker" => ['==']] | ||
| ); | ||
|
|
||
| $filteredCount = $submitter_repository->getUniqueActivitiesCountBySummit(self::$summit, $filter); | ||
|
|
||
| self::assertNotEmpty($submitterIds); | ||
| self::assertIsInt($filteredCount); | ||
| self::assertLessThanOrEqual($totalCount, $filteredCount); |
There was a problem hiding this comment.
Assert exact seeded counts here.
This still passes when getUniqueActivitiesCountBySummit() overcounts from join fan-out or applies the is_speaker filter incorrectly, because the test only checks types and broad bounds. Please pin this to the seeded summit fixture with exact expected totals for the unfiltered and filtered cases so the distinct-count behavior is actually covered.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/SubmitterRepositoryTest.php` around lines 91 - 103, The test currently
only checks types and bounds for $totalCount and $filteredCount, which lets join
fan-out or filter bugs pass; replace the loose assertions with exact expected
values for the seeded summit fixture by asserting $totalCount ===
<EXPECTED_TOTAL> and $filteredCount === <EXPECTED_FILTERED> (use the concrete
integers from the fixture), keeping the same call sites
getUniqueActivitiesCountBySummit(self::$summit, null) and
getUniqueActivitiesCountBySummit(self::$summit, $filter) and the same
FilterParser::parse([... "is_speaker" => ['=='] ...]) setup so the
distinct-count and is_speaker filter behavior is validated precisely.
smarcet
reviewed
Jun 4, 2026
| $countQb = $this->getEntityManager()->createQueryBuilder() | ||
| ->select('COUNT(DISTINCT p.id)') | ||
| ->from('models\summit\Presentation', 'p') | ||
| ->from('models\summit\PresentationSpeaker', 'e') |
Collaborator
There was a problem hiding this comment.
@mulldug
This FROM clause creates an unbounded cross-join: PresentationSpeaker is not scoped to the summit, so the query starts from a product of (presentations in summit) × (every speaker in the entire database) before the EXISTS/moderator clause narrows it down. On a large deployment — say 1K summit presentations and 10K total speakers — the engine evaluates 10M intermediate rows per call.
The member repository avoids this with a two-stage subquery: first collect matching member IDs, then count presentations for those IDs. The same pattern works here — collect matching speaker IDs in a subquery scoped to this summit's assignments, then feed that as an IN subquery into the count.
| ->select("COUNT(DISTINCT p.id)") | ||
| ->from('models\summit\Presentation', 'p') | ||
| ->where('p.summit = :summit_outer') | ||
| ->andWhere("p.created_by IN ({$qb->getDQL()})"); |
Collaborator
There was a problem hiding this comment.
@mulldug
Embedding $qb->getDQL() via string interpolation is fragile in two ways.
First, the filter mappings for this repository contain hardcoded :summit parameter references inside their EXISTS subqueries (presentations_track_id, has_accepted_presentations, presentations_title, etc.). Those parameters are propagated by the copy loop below, but if any future filter mapping introduces a parameter named :summit_outer it will silently overwrite the outer query's summit binding and return incorrect counts.
Second, getDQL() is an internal Doctrine QueryBuilder API with no stability guarantee on its output format.
Consider using Expr\In instead of string interpolation ($countQb->expr()->in('p.created_by', $qb->getDQL())) and renaming the inner query's :summit parameter to something unambiguous (e.g. :submitter_summit) to make the parameter boundary explicit.
romanetar
requested changes
Jun 4, 2026
| { | ||
| return $this->processRequest(function () use ($summit_id) { | ||
|
|
||
| $summit = SummitFinderStrategyFactory::build($this->getRepository(), $this->getResourceServerContext())->find($summit_id); |
| { | ||
| return $this->processRequest(function () use ($summit_id) { | ||
|
|
||
| $summit = SummitFinderStrategyFactory::build($this->summit_repository, $this->getResourceServerContext())->find(intval($summit_id)); |
Collaborator
There was a problem hiding this comment.
| 'full_name' => 'sometimes|string', | ||
| 'member_id' => 'sometimes|integer', | ||
| 'member_user_external_id' => 'sometimes|integer', | ||
| 'has_accepted_presentations' => 'sometimes|string|in:true,false', |
Collaborator
There was a problem hiding this comment.
@mulldug The getSpeakersActivitiesCount validation rules for has_accepted_presentations, has_alternate_presentations, and has_rejected_presentations use sometimes|required|string|in:true,false,
while the sibling getSubmittersActivitiesCount uses sometimes|string|in:true,false (no required). Since both endpoints accept the same filter semantics, the validation should be consistent
— pick one form and apply it to both.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref: https://app.clickup.com/t/86b9b1qrk
Summary by CodeRabbit
Release Notes
New Features
GET /api/v1/summits/{id}/speakers/all/events/countGET /api/v1/summits/{id}/submitters/all/events/countTests
Chores